Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix use of "-w" flag to iptables-restore #60978

Merged

Conversation

danwinship
Copy link
Contributor

iptables accepts "-w5" but iptables-restore requires "-w 5", so kube-proxy is currently broken for people with an iptables-restore new enough that kube-proxy tries to use the new flags.

Fixes #58956

Release note:

Fixed kube-proxy to work correctly with iptables 1.6.2 and later.

iptables accepts "-w5" but iptables-restore requires "-w 5"
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 9, 2018
@danwinship
Copy link
Contributor Author

ping @eparis since you're the only other person in pkg/util/iptables/OWNERS and @dcbw is on vacation right now

@danwinship
Copy link
Contributor Author

/kind bug

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Mar 9, 2018
@eparis
Copy link
Contributor

eparis commented Mar 9, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 9, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, eparis

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 9, 2018
@hoeghh
Copy link

hoeghh commented Mar 10, 2018

The problem also exist in iptables-restore v1.4.21.

[root@k8s-worker-1 vagrant]# iptables-restore --version
iptables-restore v1.4.21

[root@k8s-worker-1 vagrant]# iptables-restore -w5
iptables-restore: invalid option -- '5'

@danwinship
Copy link
Contributor Author

"Real" iptables 1.4.21 doesn't support --version or -w, and kubernetes won't try to use the flag with it.

RHEL ships iptables 1.4.21 plus a lot of backports, including the --version and -w patches. Unfortunately, iptables-restore 1.4.21 doesn't consider it a fatal error to pass an unrecognized flag (like iptables-restore 1.6.2 does); it prints an error about the "5", but it doesn't exit, and it still accepts the "-w". So that's how the bug slipped in to kubernetes; it was tested against the RHEL version of iptables, and the invalid option -- '5' message gets eaten by kubernetes if iptables-restore exits with status 0, and it was still obeying the "-w" part, so from the outside it looked like everything was working correctly.

@hoeghh
Copy link

hoeghh commented Mar 10, 2018

I changed my OS from CentOS 7 to Fedora 27 now running iptables v1.6.1. It dont get the -w5 error in journalctl, but the rules still not showing in iptables -L, though they are showing up in iptables-save.

Now i get this :

Mar 10 17:45:10 k8s-worker-1 kube-proxy[22884]: E0310 17:45:10.260208   22884 proxier.go:792] Failed to execute iptables-restore for nat: exit status 1 (iptables-restore: line 
Mar 10 17:45:10 k8s-worker-1 kube-proxy[22884]: )

Made an issue here : #61005

@thockin
Copy link
Member

thockin commented Mar 12, 2018

Am I the only one that thinks that Red Hat not changing the version number but expecting the new flag to work is somewhat wrong? Or am I misunderstanding?

@danwinship
Copy link
Contributor Author

You could argue that it's wrong, but that's totally separate from this PR (which is needed in order to support the official upstream iptables release). And the "detecting whether iptables-restore supports -w or not" part is working fine anyway. It's the "actually use -w" part that was broken.

@danwinship
Copy link
Contributor Author

This should get whatever tags, milestones, etc, are needed to make sure it ends up in 1.10.x as soon as possible after the .0 freeze ends

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-github-robot
Copy link

[MILESTONENOTIFIER] Milestone Pull Request Labels Incomplete

@danwinship @eparis

Action required: This pull request requires label changes. If the required changes are not made within 3 days, the pull request will be moved out of the v1.10 milestone.

priority: Must specify exactly one of priority/critical-urgent, priority/important-longterm or priority/important-soon.
sig owner: Must specify at least one label prefixed with sig/.

Help

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 60978, 60985). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit dce8d41 into kubernetes:master Mar 16, 2018
@adamalex
Copy link

adamalex commented Mar 23, 2018

Is there any way this could be merged into v1.9 milestone? We upgraded to v1.9.5 in order to fix some other issues, then began encountering this. Please let us know what can be done, thanks!

@danwinship danwinship deleted the fix-iptables-restore-wait branch March 26, 2018 13:30
k8s-github-robot pushed a commit that referenced this pull request Mar 31, 2018
Automatic merge from submit-queue.

Fix use of "-w" flag to iptables-restore

Backport of #60978. Will be needed by kube 1.9 users who upgrade to iptables 1.6.2. (Older kube releases had different code and don't need the backport.)

**Release note**:
```release-note
Fixed kube-proxy to work correctly with iptables 1.6.2 and later.
```
@kumudt
Copy link

kumudt commented Apr 21, 2018

I am using kops debain stretch image which has iptables 1.6.0 and even after upgrading the cluster to 1.9.7 which has this fix backported the issue still remains. I still see the error in the logs and the services present on the node started crashing with liveness failures.

@danwinship
Copy link
Contributor Author

iptables 1.6.0 does not need this fix. you presumably have a different problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. milestone/incomplete-labels release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants